-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
riscv64: Optimize storing zero to memory by using zero_reg
#8719
riscv64: Optimize storing zero to memory by using zero_reg
#8719
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat!
I also have a suspicion that the CI failures might be that we are now trying to encode a compressed store with the zero register. I don't remember if this is legal, but I'm going to investigate it a bit.
Edit: Yeah, the RISC-V Manual (Page 70) states:
For many RVC instructions, zero-valued immediates are disallowed and x0 is not a valid 5-bit register specifier. These restrictions free up encoding space for other instructions requiring fewer operand bits.
There is nothing specific about the store instructions falling under these restrictions and It's probably why we don't have the check here. This still allows us to use the zero register for floating point compressed stores though!
Note the regular stores (non-stack based) should not need an update since they use a compressed register which automatically excludes the zero register.
At least one issue I tracked to my misunderstanding of how |
This commit is similar to bytecodealliance#8701 in that it adds a special case to `store` operations to use the `zero` register when applicable.
aef60c2
to
17ad1f7
Compare
Ok I did a bit of an audit of the compressed stores. I added a new test in the zcb.clif test doing all the store widths to both an arbitrary register and the stack. My bugfix around my misinterpretation of Compressed loads to arbitrary registers already don't work since that gates on the source being x8-x15 (as you pointed out). We are emitting compressed loads to the stack for
By this do you mean that we could use a compressed |
Nice! Good to know.
For loads we shouldn't be, at least we have that specific check in the emit code For stores I would agree, If capstone disassembles it it's probably intended to work. It also seems like a very useful instruction to have, so I wouldn't be surprised that it is intended.
I think this is fine as is. I'm fairly sure that if it wasn't intended the testsuites would fail in a bunch of places.
No, just that the zero_reg in the emit for
That's super weird, and I would not expect that at all. |
Oops sorry I misspoke, I meant stores! |
This commit is similar to #8701 in that it adds a special case to
store
operations to use thezero
register when applicable.